Feat/paymaster foundry tests #1

ShivaanshK · 2024-07-01T14:28:09Z

Unit tests for BiconomySponsorshipPaymasterWithPremium
Fuzz tests for BiconomySponsorshipPaymasterWithPremium

Fix/project build

Aboudjem

To enhance the readability and cleanliness of your code I think you should try breaking up large blocks with new lines where appropriate. This makes the code easier to read and understand

test/foundry/base/NexusTestBase.sol

Aboudjem · 2024-07-03T07:43:32Z

test/foundry/base/NexusTestBase.sol

+        vm.prank(FACTORY_OWNER.addr);
+        META_FACTORY.addFactoryToWhitelist(address(FACTORY));
+        HANDLER_MODULE = new MockHandler();
+        // EXECUTOR_MODULE = new MockExecutor();


remove commented code

Aboudjem · 2024-07-03T07:46:02Z

test/foundry/base/NexusTestBase.sol

+    NexusAccountFactory internal FACTORY;
+    BiconomyMetaFactory internal META_FACTORY;
+    MockHandler internal HANDLER_MODULE;
+    // MockExecutor internal EXECUTOR_MODULE;


remove commented code

Aboudjem · 2024-07-03T07:48:28Z

test/foundry/base/NexusTestBase.sol

+    event OwnershipTransferred(address indexed oldOwner, address indexed newOwner);
+


move this to EventsAndErrors

Aboudjem · 2024-07-03T07:48:37Z

test/foundry/base/NexusTestBase.sol

+import { BiconomyMetaFactory } from "nexus/contracts/factory/BiconomyMetaFactory.sol";
+import { MockValidator } from "nexus/contracts/mocks/MockValidator.sol";
+import { MockHook } from "nexus/contracts/mocks/MockHook.sol";
+// import { MockExecutor } from "nexus/contracts/mocks/MockExecutor.sol";


remove this

Aboudjem · 2024-07-03T08:03:34Z

test/foundry/unit/concrete/TestSponsorshipPaymasterWithPremiumTest.t.sol

+    function test_SetVerifyingSigner() external {
+        vm.startPrank(PAYMASTER_OWNER.addr);
+        vm.expectEmit(true, true, true, true, address(bicoPaymaster));
+        emit IBiconomySponsorshipPaymaster.VerifyingSignerChanged(


@livingrockrises we should change the name of the methods / funcs that contains verifying as it is the old name and we changed it since then

it is still verifyingSigner!
paymaster name change to sponsorship paymaster is good
but what do you suggest then for this verifyingSigner?

Aboudjem · 2024-07-03T08:05:06Z

test/foundry/unit/concrete/TestSponsorshipPaymasterWithPremiumTest.t.sol

+    }
+
+    function test_RevertIf_UnauthorizedSetVerifyingSigner() external {
+        vm.startPrank(DAN_ADDRESS);


prank here is not really necessary

you can call it directly imo

Aboudjem · 2024-07-03T08:05:36Z

test/foundry/unit/concrete/TestSponsorshipPaymasterWithPremiumTest.t.sol

+    function test_SetFeeCollector() external {
+        vm.startPrank(PAYMASTER_OWNER.addr);
+        vm.expectEmit(true, true, true, true, address(bicoPaymaster));
+        emit IBiconomySponsorshipPaymaster.FeeCollectorChanged(
+            PAYMASTER_FEE_COLLECTOR.addr, DAN_ADDRESS, PAYMASTER_OWNER.addr
+        );
+        bicoPaymaster.setFeeCollector(DAN_ADDRESS);
+        assertEq(bicoPaymaster.feeCollector(), DAN_ADDRESS);
+        vm.stopPrank();


check initial state

Aboudjem · 2024-07-03T08:05:43Z

test/foundry/unit/concrete/TestSponsorshipPaymasterWithPremiumTest.t.sol

+    function test_RevertIf_SetFeeCollectorToZeroAddress() external {
+        vm.startPrank(PAYMASTER_OWNER.addr);
+        vm.expectRevert(abi.encodeWithSignature("FeeCollectorCannotBeZero()"));
+        bicoPaymaster.setFeeCollector(address(0));
+        vm.stopPrank();
+    }


Aboudjem · 2024-07-03T08:06:02Z

test/foundry/unit/concrete/TestSponsorshipPaymasterWithPremiumTest.t.sol

+    function test_RevertIf_UnauthorizedSetFeeCollector() external {
+        vm.startPrank(DAN_ADDRESS);
+        vm.expectRevert(abi.encodeWithSignature("Unauthorized()"));
+        bicoPaymaster.setFeeCollector(DAN_ADDRESS);
+        vm.stopPrank();
+    }


same applies for all Unauthorized errors

Aboudjem · 2024-07-03T08:16:39Z

there are errors on checks, please fix compiler issues and test issues

Aboudjem · 2024-07-03T08:19:07Z

since you removed Lock consider also removing the test file on hardhat test

Feat/fix linting

…my/gasdaddy into feat/paymaster-foundry-tests

livingrockrises · 2024-07-10T07:19:43Z

Can you also add a test for parsePaymasterAndData

livingrockrises · 2024-07-10T07:27:53Z

test/foundry/unit/concrete/TestSponsorshipPaymasterWithPremiumTest.t.sol

+        assertEq(address(testArtifact.entryPoint()), ENTRYPOINT_ADDRESS);
+        assertEq(testArtifact.verifyingSigner(), PAYMASTER_SIGNER.addr);
+        assertEq(testArtifact.feeCollector(), PAYMASTER_FEE_COLLECTOR.addr);
+        assertEq(testArtifact.postOpCost(), 0 wei);


let's set some postOpCost upon deployment..

livingrockrises · 2024-07-10T07:30:43Z

test/foundry/unit/concrete/TestSponsorshipPaymasterWithPremiumTest.t.sol

+        bicoPaymaster.setPostopCost(newPostopCost);
+    }
+
+    function test_DepositFor() external {


just remembered depositFor when you do first time it will be cold access on paymasterIdBalances
and second time it will be warm.
so with different dapp accounts tests should keep this in mind. accounting wise too

livingrockrises

review i
looks good.
on naming convention of some methods may have some inputs in a bit

livingrockrises · 2024-07-10T07:33:47Z

check failing CI flows as well, take help of Adam later

…change

livingrockrises

LGTM

ShivaanshK and others added 16 commits June 26, 2024 13:13

Setup nexus submodule and fix remappings for foundry test

55d967d

setup imports, accounts, and align versions

659b7e3

Deployment test for BiconomySponsorshipPaymaster

967379c

basic state tests

2e32ee2

quick fix

46d8c89

deposit tests

87c8c69

rename tests according to naming conventions

6a85282

Check events being emitted

0eb2f81

tests for withdrawing from paymaster

502ee33

tests for validating paymaster and postop

d7efb30

tests for receiving and withdrawing ether to/from paymaster

03ff890

commit unit tests

404b493

fuzz tests

4ba40d4

all fuzz tests

5868a5c

get rid of nonfuzz test in fuzz file

2fd121c

forge install: nexus

45c20eb

ShivaanshK assigned livingrockrises Jul 1, 2024

build dep fixes

d01ace0

livingrockrises requested review from livingrockrises and Aboudjem July 2, 2024 11:28

Merge pull request #2 from bcnmy/fix/project-build

d97036f

Fix/project build

Aboudjem suggested changes Jul 3, 2024

View reviewed changes

add adam's requested changes

5da6fbe

ShivaanshK requested a review from Aboudjem July 3, 2024 11:55

ShivaanshK assigned Aboudjem Jul 3, 2024

ShivaanshK and others added 3 commits July 3, 2024 17:41

fixed linting and changed visibility where applicable

b4276e4

yarn lint runs successfully

9e72f5c

Merge pull request #5 from bcnmy/feat/fix-linting

c62faf2

Feat/fix linting

livingrockrises and others added 13 commits July 4, 2024 16:25

fix forge build

38f9891

add adam's requested changes

3ac0568

setPostOpCost tests

de22654

fixed linting and changed visibility where applicable

3972d14

yarn lint runs successfully

f4f341c

tests for withdrawErc20

8326d90

fix forge build

1ee053e

switched to custom errors and fixed postop gas calc

e1ba22e

fixed a few things with gas calc and wrote tests

10b3892

more tests for accounting for premiums

2817e44

some cleanup

1d96053

make tests more modular

40aed7e

postop cost added back and more tests added

81abba5

ShivaanshK force-pushed the feat/paymaster-foundry-tests branch from 31a4ea1 to 81abba5 Compare July 9, 2024 13:07

livingrockrises added 2 commits July 9, 2024 18:47

Merge branch 'feat/paymaster-foundry-tests' of https://github.com/bcn…

4ba9a4b

…my/gasdaddy into feat/paymaster-foundry-tests

fix postOpCost

abff5d2

livingrockrises reviewed Jul 10, 2024

View reviewed changes

ShivaanshK added 4 commits July 10, 2024 12:21

parse paymaster data test

f5acd1a

comprehensive accounting tests

ebf05c1

fix-lint

36d854b

make UNACCOUNTED_GAS_LIMIT a constant

d59cfd5

ShivaanshK requested a review from livingrockrises July 10, 2024 14:25

added check for bundler balance change equal to paymaster EP balance …

358aeb1

…change

livingrockrises approved these changes Jul 11, 2024

View reviewed changes

livingrockrises merged commit 036840b into develop Jul 11, 2024
1 of 3 checks passed

livingrockrises deleted the feat/paymaster-foundry-tests branch October 7, 2024 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/paymaster foundry tests #1

Feat/paymaster foundry tests #1

ShivaanshK commented Jul 1, 2024

Aboudjem left a comment

Aboudjem Jul 3, 2024

Aboudjem Jul 3, 2024

Aboudjem Jul 3, 2024

Aboudjem Jul 3, 2024

Aboudjem Jul 3, 2024

livingrockrises Jul 10, 2024

Aboudjem Jul 3, 2024

Aboudjem Jul 3, 2024

Aboudjem Jul 3, 2024

Aboudjem Jul 3, 2024

Aboudjem Jul 3, 2024

Aboudjem commented Jul 3, 2024

Aboudjem commented Jul 3, 2024

livingrockrises commented Jul 10, 2024

livingrockrises Jul 10, 2024

livingrockrises Jul 10, 2024

livingrockrises left a comment

livingrockrises commented Jul 10, 2024

livingrockrises left a comment

		event OwnershipTransferred(address indexed oldOwner, address indexed newOwner);

Feat/paymaster foundry tests #1

Feat/paymaster foundry tests #1

Conversation

ShivaanshK commented Jul 1, 2024

Aboudjem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aboudjem commented Jul 3, 2024

Aboudjem commented Jul 3, 2024

livingrockrises commented Jul 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

livingrockrises left a comment

Choose a reason for hiding this comment

livingrockrises commented Jul 10, 2024

livingrockrises left a comment

Choose a reason for hiding this comment